Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enhance DrawingView and DrawingViewService to provide the ability to export the full image size #2193

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Enhance DrawingView and DrawingViewService to provide the ability to export the full image size #2193

wants to merge 22 commits into from

Conversation

bijington
Copy link
Contributor

@bijington bijington commented Sep 10, 2024 •
edited
Loading

Description of Change

The aim of this PR is to allow developers to export their drawings in multiple ways:

  • The current approach which only exports the lines with a tiny amount of padding
  • The full canvas to fully represent what was on screen

The following image shows the result in the Files app in the simulator on iOS based on using the sample application from the toolkit.

Simulator Screenshot - iPhone 15 - 2024-09-10 at 06 38 55

The change allows a developer to provide an ImageOutputOption value to the DrawingView:

/// <summary>
/// Enumeration of the options available when generating an image stream using the DrawingView.
/// </summary>
public enum DrawingViewOutputOption
{
	/// <summary>
	/// Outputs the area covered by the top-left to the bottom-right most points.
	/// </summary>
	Lines,
	
	/// <summary>
	/// Outputs the full area displayed within the drawing view.
	/// </summary>
	FullCanvas
}

This will then pass into the DrawingViewService the bounds of the current DrawingView. Developers can then also supply their out dimensions should they wish to the DrawingViewService.

Linked Issues

PR Checklist

Additional information

@bijington
Copy link
Contributor Author

@VladislavAntonyuk what do you think to this approach to enable developers to export the whole canvas should they wish?

It needs to be completed for all platforms but I have the PoC working for Apple platforms

@bijington bijington mentioned this pull request Sep 10, 2024
Open
2 tasks
@VladislavAntonyuk
Copy link
Collaborator

Thanks @bijington , looks good. What do you think about renaming imageSize to lineBounds? Because ImageSize and CanvasSize may be confused.

@bijington
Copy link
Contributor Author

@VladislavAntonyuk great idea! I will carry on then and include that suggestion thanks.

Also sadly I can't work out how to make this non breaking so I'll add the breaking label

@bijington bijington added the breaking change This label is used for PRs that include a breaking change label Sep 10, 2024
@VladislavAntonyuk
Copy link
Collaborator

To make it not a breaking change, we can move the canvasSize parameter to the last position, but it's not good to have something after CancellationToken. However, I don't think it's a problem as we had breaking changes before. We can probably merge it with .net 9 support

@bijington
Copy link
Contributor Author

To make it not a breaking change, we can move the canvasSize parameter to the last position, but it's not good to have something after CancellationToken. However, I don't think it's a problem as we had breaking changes before. We can probably merge it with .net 9 support

I did originally try that but one of our analysis rules complained as it is 'best practise' to keep the CancellationToken parameter at the end.

@bijington
Copy link
Contributor Author

Or I guess rather than a default parameter we have an overload and therefore the current behaviour will stay as the default... That might work

@bijington bijington removed the breaking change This label is used for PRs that include a breaking change label Sep 10, 2024
@bijington
Copy link
Contributor Author

The latest push removes the breaking nature of this PR

@bijington bijington marked this pull request as ready for review September 10, 2024 14:11
@bijington
Copy link
Contributor Author

FYI I haven't tested Android, Windows or Tizen yet. The last 2 I wrote on a Mac so I don't even know if it compiles yet...

@bijington
Copy link
Contributor Author

I am unable to test on Windows or Tizen right now as my home network is down while we do some renovations. If anyone can test either platform that would be really helpful. Otherwise I will move onto the docs

@bijington bijington requested a review from a team September 10, 2024 19:57
@bijington bijington mentioned this pull request Sep 10, 2024
Draft
@bijington bijington changed the title Initial attempt at exposing the ability to export the full image size Enhance DrawingView and DrawingViewService to provide the ability to export the full image size Sep 11, 2024
VladislavAntonyuk
VladislavAntonyuk reviewed Oct 30, 2024
View reviewed changes
src/CommunityToolkit.Maui.Core/Views/DrawingView/Service/DrawingViewService.shared.cs
/// <param name="background">Image background</param>
/// <param name="token"><see cref="CancellationToken"/></param>
/// <returns>Image stream</returns>
public static ValueTask<Stream> GetImageStream(IList<PointF> points, Size desiredSize, float lineWidth, Color strokeColor, Paint? background, CancellationToken token = default) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about moving parameters in Options class?
So new method signature looks like this:

Suggested change
public static ValueTask<Stream> GetImageStream(IList<PointF> points, Size desiredSize, float lineWidth, Color strokeColor, Paint? background, CancellationToken token = default) =>
public static ValueTask<Stream> GetImageStream(IList<PointF> points, ImageOptions options, CancellationToken token = default)

In this case we may easily extend image options without breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladislavAntonyuk VladislavAntonyuk VladislavAntonyuk left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DrawingView.GetImageStream Size has no affect
2 participants
@bijington @VladislavAntonyuk